-
Notifications
You must be signed in to change notification settings - Fork 28
Address binskim flag for compiler in driver build #99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: npu/release/19.x
Are you sure you want to change the base?
Address binskim flag for compiler in driver build #99
Conversation
Add AEK_PAUTH to ARMV8_3A in TargetParser and let it propagate to ARMV8R, as it aligns with GCC defaults. After adding AEK_PAUTH, several tests from TargetParserTest.cpp crashed when trying to format an error message, thus update a format string in AssertSameExtensionFlags to account for bitmask being pre-formatted as std::string. The CHECK-PAUTH* lines in aarch64-target-features.c are updated to account for the fact that FEAT_PAUTH support and pac-ret can be enabled independently and all four combinations are possible. (cherry picked from commit a52eea6)
The Ampere1B is Ampere's third-generation core implementing a superscalar, out-of-order microarchitecture with nested virtualization, speculative side-channel mitigation and architectural support for defense against ROP/JOP style software attacks. Ampere1B is an ARMv8.7+ implementation, adding support for the FEAT WFxT, FEAT CSSC, FEAT PAN3 and FEAT AFP extensions. It also includes all features of the second-generation Ampere1A, such as the Memory Tagging Extension and SM3/SM4 cryptography instructions. (cherry picked from commit fbba818)
The Ampere1B core is enabled with a new scheduling/pipeline model, as it provides significant updates over the Ampere1 core; it reduces latencies on many instructions, has some micro-ops reassigned between the XY and X units, and provides modelling for the instructions added since Ampere1 and Ampere1A. As this is the first model implementing the CSSC instructions, we update the UnsupportedFeatures on all other models (that have CompleteModel set). Testcases are added under llvm-mca: these showed the FullFP16 feature missing, so we are adding it in as part of this commit. This *adds tests and additional fixes* compared to the reverted #81338. (cherry picked from commit dd1897c)
One of the whitespace fixes didn't get added to the commit introducing the Ampere1B model. Clean it up. (cherry picked from commit 3369e34)
…… (#77291) …n MIPS Modify: Add a global variable 'CurForbiddenSlotAttr' to save current instruction's forbidden slot and whether set reorder. This is the judgment condition for whether to add nop. We would add a couple of '.set noreorder' and '.set reorder' to wrap the current instruction and the next instruction. Then we can get previous instruction`s forbidden slot attribute and whether set reorder by 'CurForbiddenSlotAttr'. If previous instruction has forbidden slot and .set reorder is active and current instruction is CTI. Then emit a NOP after it. Fix llvm/llvm-project#61045. Because https://reviews.llvm.org/D158589 was 'Needs Review' state, not ending, so we commit pull request again. (cherry picked from commit 96abee5)
This include 2 fixes: 1. Disallow 'f' for softfloat. 2. Allow 'r' for softfloat. Currently, 'f' is accpeted by clang, then LLVM meets an internal error. 'r' is rejected by LLVM by: couldn't allocate input reg for constraint 'r'. Fixes: #64241, #63632 --------- Co-authored-by: Fangrui Song <[email protected]> (cherry picked from commit c88beb4)
The modules used is-standard-library and is-std-library. The latter is the name used in the SG15 proposal, Fixes: llvm/llvm-project#82879 (cherry picked from commit b50bcc7)
In gas, .cpsetup may expand to one of two code sequences (one is related to `__gnu_local_gp`), depending on -mno-shared and -msym32. Since Clang doesn't support -mno-shared or -msym32, .cpsetup expands to one code sequence. The N32 condition incorrectly leads to the incorrect `__gnu_local_gp` code sequence. ``` 00000000 <t1>: 0: ffbc0008 sd gp,8(sp) 4: 3c1c0000 lui gp,0x0 4: R_MIPS_HI16 __gnu_local_gp 8: 279c0000 addiu gp,gp,0 8: R_MIPS_LO16 __gnu_local_gp ``` Fixes: #52785 (cherry picked from commit 860b6ed)
A symbol with an `N_ALT_ENTRY` attribute may be defined in the middle of a subsection, so it is reasonable to opt them out of the `.cfi_{start,end}proc` nesting check. Fixes: llvm/llvm-project#82261 (cherry picked from commit 5b91647)
… (#83159) In addition to being rather hard to follow, there isn't a good reason why FatLTO shouldn't just share the same code for setting module flags for (Thin)LTO. This patch simplifies the logic and makes sure we use set these flags in a consistent way, independent of FatLTO. Additionally, we now test that output in the .llvm.lto section actually matches the output from Full and Thin LTO compilation. (cherry picked from commit 7d8b50a)
DSE uses BatchAA, which caches queries using pairs of MemoryLocations. At the moment, DSE may remove instructions that are used as pointers in cached MemoryLocations. If a new instruction used by a new MemoryLoation and this instruction gets allocated at the same address as a previosuly cached and then removed instruction, we may access an incorrect entry in the cache. To avoid this delay removing all instructions except MemoryDefs until the end of DSE. This should avoid removing any values used in BatchAA's cache. Test case by @vporpo from llvm/llvm-project#83181. (Test not precommitted because the results are non-determinstic - memset only sometimes gets removed) PR: llvm/llvm-project#83411 (cherry picked from commit 10f5e98)
glibc 2.39 added `nonnull` attribute to most libio functions accepting a `FILE*` parameter, including fprintf[1]. The -fsanitize=undefined mode checks the argument to fprintf and has extra counters, not expected by two tests. Specify -fno-sanitize=nonnull-attribute to make the two tests pass. Fix #82883 [1]: https://sourceware.org/git/?p=glibc.git;a=commit;h=64b1a44183a3094672ed304532bedb9acc707554 Pull Request: llvm/llvm-project#84231 (cherry picked from commit c3acbf6)
Fixes #84025. (cherry picked from commit e96c0c1)
When replacing with a non-constant, it's possible that the result of the simplification is actually more complicated than the original, and may result in an infinite combine loop. Mitigate the issue by requiring that either the replacement or simplification result is constant, which should ensure that it's simpler. While this check is crude, it does not appear to cause optimization regressions in real-world code in practice. Fixes llvm/llvm-project#83127. (cherry picked from commit 9f45c5e)
… (#84540) MSVC does not define __BYTE_ORDER__ making the check for BigEndian erroneously evaluate to true and breaking the struct definitions in MSVC compiled builds correspondingly. The fix adds an additional check for whether __BYTE_ORDER__ is defined by the compiler to fix these. --------- Co-authored-by: Vadim Paretsky <[email protected]> (cherry picked from commit 110141b)
…e (#83990) We should generate the `MCInstPredicate` twice, one with `FirstMI` and another with `SecondMI`. (cherry picked from commit de1f338)
…apArg (#83656) This patch aims to solve Firefox issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1882301 Similar to 616289e. Currently LoongArch uses an ll.[wd]/sc.[wd] loop for ATOMIC_CMP_XCHG. Because the comparison in the loop is full-width (i.e. the `bne` instruction), we must sign extend the input comparsion argument. Note that LoongArch ISA manual V1.1 has introduced compare-and-swap instructions. We would change the implementation (return `ANY_EXTEND`) when we support them. (cherry picked from commit 5f058aa)
ARM64EC varargs calls expect that x4 = sp at entry, special handling is needed to ensure this with tail calls since they occur after the epilogue and the x4 write happens before. I tried going through AArch64MachineFrameLowering for this, hoping to avoid creating the dummy object but this was the best I could do since the stack info that uses isn't populated at this stage, CreateFixedObject also explicitly forbids 0 sized objects.
…s (#80595) ISel handles filling in x4/x5 when calling variadic functions as they don't correspond to the 5th/6th X64 arguments but rather to the end of the shadow space on the stack and the size in bytes of all stack parameters (ignored and written as 0 for calls from entry thunks). Will PR a follow up with ISel handling after this is merged.
…ls (#80994) When in an entry thunk the x64 SP is passed in x4 but this cannot be directly passed through since x64 varargs calls have a 32 byte shadow store at SP followed by the in-stack parameters. ARM64EC varargs calls on the other hand expect x4 to point to the first in-stack parameter.
This re-lands cc0065a in a way that keeps existing targets working. --------- Original commit message: #68132 ended up removing __multc3 & __divtc3 from compiler-rt library builds that have QUAD_PRECISION but not TF_MODE due to missing int128 support. I added support for QUAD_PRECISION to use the native hex float long double representation. --------- Co-authored-by: Sean Perry <[email protected]> (cherry picked from commit 99c457d)
g++ -flto has a diagnostic `-Wodr` about mismatched redeclarations, which even apply to `enum`. Fix #83529 Reviewers: thesamesam Reviewed By: thesamesam Pull Request: llvm/llvm-project#83604 (cherry picked from commit 4a3f7e7)
…(#83384) When MVT is not a vector type, TCK_CodeSize should return an invalid cost. This patch adds a check in the beginning to make sure all cost kinds return invalid costs consistently. Before this patch, TCK_CodeSize returns a valid cost on scalar MVT but other cost kinds doesn't. This fixes the issue #83294 where a loop contains vector instructions and MVT is scalar after type legalization when the vector extension is not enabled, (cherry picked from commit fb67dce)
When analysing whether we should handle a binary expression as an overloaded operator call or a builtin operator, we were calling `checkPlaceholderForOverload()`, which takes care of any placeholders that are not overload sets—which would usually make sense since those need to be handled as part of overload resolution. Unfortunately, we were also doing that for `.*`, which is not overloadable, and then proceeding to create a builtin operator anyway, which would crash if the RHS happened to be an unresolved overload set (due hitting an assertion in `CreateBuiltinBinOp()`—specifically, in one of its callees—in the `.*` case that makes sure its arguments aren’t placeholders). This pr instead makes it so we check for *all* placeholders early if the operator is `.*`. It’s worth noting that, 1. In the `.*` case, we now additionally also check for *any* placeholders (not just non-overload-sets) in the LHS; this shouldn’t make a difference, however—at least I couldn’t think of a way to trigger the assertion with an overload set as the LHS of `.*`; it is worth noting that the assertion in question would also complain if the LHS happened to be of placeholder type, though. 2. There is another case in which we also don’t perform overload resolution—namely `=` if the LHS is not of class or enumeration type after handling non-overload-set placeholders—as in the `.*` case, but similarly to 1., I first couldn’t think of a way of getting this case to crash, and secondly, `CreateBuiltinBinOp()` doesn’t seem to care about placeholders in the LHS or RHS in the `=` case (from what I can tell, it, or rather one of its callees, only checks that the LHS is not a pseudo-object type, but those will have already been handled by the call to `checkPlaceholderForOverload()` by the time we get to this function), so I don’t think this case suffers from the same problem. This fixes #53815. --------- Co-authored-by: Aaron Ballman <[email protected]>
…es (#81133) For a label difference like `.uleb128 A-B`, MC generates a pair of R_LARCH_{ADD,SUB}_ULEB128 if A-B cannot be folded as a constant. GNU assembler generates a pair of relocations in more cases (when A or B is in a code section with linker relaxation). It is similar to RISCV. R_LARCH_{ADD,SUB}_ULEB128 relocations are created by Clang and GCC in `.gcc_except_table` and other debug sections with linker relaxation enabled. On LoongArch, first read the buf and count the available space. Then add or sub the value. Finally truncate the expected value and fill it into the available space. (cherry picked from commit eaa9ef6)
…AndConstant` (cherry picked from commit d51fcd4)
I confirm there are no errors regarding suppressed warnings in BinSkim scan |
@@ -767,6 +767,15 @@ if (MSVC) | |||
foreach(flag ${msvc_warning_flags}) | |||
append("${flag}" CMAKE_C_FLAGS CMAKE_CXX_FLAGS) | |||
endforeach(flag) | |||
|
|||
if (BUILD_COMPILER_FOR_DRIVER) | |||
string(REPLACE "-wd4146" "" CMAKE_C_FLAGS "${CMAKE_C_FLAGS}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I'm not quite familiar with LLVM CMake code involved here, but I'd consider updating global CMAKE variables, especially CMAKE_C/CXX_* not the best practice, reason is there's a chance it'd affect other targets you didn't intended to change
I saw that case in our project already, when OpenVINO did this and then propagated not always desired compilation options to our targets
If you stick with modifying these variables for some reason, could you please clarify why and confirm no unintended target is changed (presumably nothing outside of llvm-project folder)?
Alternative would be just working with target properties, you can specify compilation options per target such it's not intrusive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to modify and test based on your suggestion.
…l#96) Bumps the github-actions group with 8 updates in the / directory: | Package | From | To | | --- | --- | --- | | [step-security/harden-runner](https://github.com/step-security/harden-runner) | `2.10.1` | `2.10.4` | | [actions/checkout](https://github.com/actions/checkout) | `4.1.1` | `4.2.2` | | [actions/dependency-review-action](https://github.com/actions/dependency-review-action) | `4.4.0` | `4.5.0` | | [tj-actions/changed-files](https://github.com/tj-actions/changed-files) | `45.0.3` | `45.0.6` | | [aminya/setup-cpp](https://github.com/aminya/setup-cpp) | `0.44.0` | `0.46.0` | | [actions/setup-python](https://github.com/actions/setup-python) | `5.3.0` | `5.4.0` | | [ossf/scorecard-action](https://github.com/ossf/scorecard-action) | `2.3.1` | `2.4.0` | | [actions/upload-artifact](https://github.com/actions/upload-artifact) | `97a0fba1372883ab732affbe8f94b823f91727db` | `c24449f33cd45d4826c6702db7e49f7cdb9b551d` | Updates `step-security/harden-runner` from 2.10.1 to 2.10.4 - [Release notes](https://github.com/step-security/harden-runner/releases) - [Commits](step-security/harden-runner@91182cc...cb605e5) Updates `actions/checkout` from 4.1.1 to 4.2.2 - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4.1.1...11bd719) Updates `actions/dependency-review-action` from 4.4.0 to 4.5.0 - [Release notes](https://github.com/actions/dependency-review-action/releases) - [Commits](actions/dependency-review-action@4081bf9...3b139cf) Updates `tj-actions/changed-files` from 45.0.3 to 45.0.6 - [Release notes](https://github.com/tj-actions/changed-files/releases) - [Changelog](https://github.com/tj-actions/changed-files/blob/main/HISTORY.md) - [Commits](tj-actions/changed-files@c3a1bb2...d6e91a2) Updates `aminya/setup-cpp` from 0.44.0 to 0.46.0 - [Release notes](https://github.com/aminya/setup-cpp/releases) - [Commits](aminya/setup-cpp@d485b24...12e62a1) Updates `actions/setup-python` from 5.3.0 to 5.4.0 - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@0b93645...4237552) Updates `ossf/scorecard-action` from 2.3.1 to 2.4.0 - [Release notes](https://github.com/ossf/scorecard-action/releases) - [Changelog](https://github.com/ossf/scorecard-action/blob/main/RELEASE.md) - [Commits](ossf/scorecard-action@0864cf1...62b2cac) Updates `actions/upload-artifact` from 97a0fba1372883ab732affbe8f94b823f91727db to c24449f33cd45d4826c6702db7e49f7cdb9b551d - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@97a0fba...c24449f) --- updated-dependencies: - dependency-name: step-security/harden-runner dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github-actions - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-minor dependency-group: github-actions - dependency-name: actions/dependency-review-action dependency-type: direct:production update-type: version-update:semver-minor dependency-group: github-actions - dependency-name: tj-actions/changed-files dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github-actions - dependency-name: aminya/setup-cpp dependency-type: direct:production update-type: version-update:semver-minor dependency-group: github-actions - dependency-name: actions/setup-python dependency-type: direct:production update-type: version-update:semver-minor dependency-group: github-actions - dependency-name: ossf/scorecard-action dependency-type: direct:production update-type: version-update:semver-minor dependency-group: github-actions - dependency-name: actions/upload-artifact dependency-type: direct:production dependency-group: github-actions ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
llvm/lib/Support/CMakeLists.txt
Outdated
if(MSVC) | ||
if(BUILD_COMPILER_FOR_DRIVER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: if (MSVC AND BUILD_COMPILER_FOR_DRIVER)
Also, since we have to modified LLVM's codebase, I suppose we should have some kind tracker of all such changes
@nikita-kud do we have anything like this already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikita-kud do we have anything like this already?
Yes, we have: E-101222
But I think it can't be upstreamed. This change looks sad. Why we can't suppress these warnings scanner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikita-kud do we have anything like this already?
Yes, we have: E-101222 But I think it can't be upstreamed. This change looks sad. Why we can't suppress these warnings scanner?
Because we need to pass binskim. @AlexandruLorinti We are currently fix the binskim scan failures by applying a patch during the CiD build. Since it cannot be upstreamed, can we continue using this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikita-kud do we have anything like this already?
Yes, we have: E-101222 But I think it can't be upstreamed. This change looks sad. Why we can't suppress these warnings scanner?
Because we need to pass binskim. @AlexandruLorinti We are currently fix the binskim scan failures by applying a patch during the CiD build. Since it cannot be upstreamed, can we continue using this method?
any method would be ok, you can keep the patch in CiD build
85abebf
to
cb1282d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment to each modification of compile options
Maybe makes sense to create a CMake function that accepts target and silences all needed warnings, then you could add explanation to just function implementation
In this case all the logic for making binskim happy is reused
llvm/lib/Analysis/CMakeLists.txt
Outdated
|
||
if(MSVC AND BUILD_COMPILER_FOR_DRIVER) | ||
target_compile_options(LLVMAnalysis PRIVATE | ||
/w34146 /w34244 /w34267 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to do the same in NPU Cmake which includes LLVM, avoiding a patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I guess it's true, we could write CMake code/function in our project that processes pre-defined set of targets (from LLVM repo that we know are problematic to one our jobs/processes) and for each given target modifies a property of it that corresponds to compile options used to build the target accordingly
This way we get clean separation - this warnings disablement is not a problem of LLVM (since they can build their targets they probably just do not provide support for these-warnings-clean-build), but our problem since we seem to place more requirements on LLVM codebase than its authors/maintainers
llvm/lib/Analysis/CMakeLists.txt
Outdated
# Due to this target cannot pass the binskim scan after the compiler in the driver build, | ||
# it is necessary to remove the suppression of these three warnings(/wd4146 /wd4244 /wd4267). | ||
enable_msvc_warnings_on_compiler_for_driver(LLVMAnalysis /w3 4146 4244 4267) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move comments to the function implementation, no need to repeat every time you call the function
Function name is confusing, do you disable or enable warnings here? From the values looks like you disable them
Also, I'd keep function specific to your use case, instead of accepting warnings as arguments, just "hardcode" them internally and name the function something along the lines as fix_binksim or something (choose better name than mine!)
Also consider @lmielick's comment to implement solution in compiler repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thank you for your suggestions! Here we are enabling warnings, and I will modify the function according to your advice. Additionally, I am trying to modify only the compiler repo to make it pass binskim.
Bumps [cryptography](https://github.com/pyca/cryptography) from 43.0.1 to 44.0.1. - [Changelog](https://github.com/pyca/cryptography/blob/main/CHANGELOG.rst) - [Commits](pyca/cryptography@43.0.1...44.0.1) --- updated-dependencies: - dependency-name: cryptography dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Security fixes ww11 * Add tar.gz jinja sha256
Signed-off-by: Zhu, Shaojie <[email protected]>
26236fc
to
5ce36b2
Compare
Firstly, can not directly enable warnings for LLVM within the compiler repo. Secondly, I tried creating a function in the compiler repo and calling this function for each target in LLVM that fails the binskim check. This approach reduces the binskim errors, but it involves modifying many targets. Additionally, if the same errors appear in other targets in the future, further modifications to the LLVM code will be necessary. Therefore, I believe it is more appropriate to add an option in LLVM that allows controlling the enabling of relevant warnings from the compiler repo. |
Test job: https://jenkins-npu-compiler.ir.intel.com/job/pub/job/IE-Packages/job/Release/job/CiD-Windows10/15966/ passed |
Why do we need it in LLVM? Can we instead do it at the compiler's side around Also, removing warnings doesn't look good to me. Are there issues in LLVM otherwise? Can we fix them instead and push that to upstream? |
@ShaojieZhuIntel did you try using directory level CMAKE variables? I would presume we can append these flags into CFLAGS on directory level. Even if this is applied eagerly for all LLVM targets it should be fine: |
We are not removing warnings; we are enabling warnings because some warnings are suppressed in LLVM, which is not allowed by binskim. Like:
Even if we handle this around add_subdirectory(LLVM), it will be overridden by the actions in LLVM that disable warnings.
-wd4146 # Suppress 'unary minus operator applied to unsigned type, result still unsigned'
-wd4244 # Suppress ''argument' : conversion from 'type1' to 'type2', possible loss of data'
-wd4267 # Suppress ''var' : conversion from 'size_t' to 'type', possible loss of data' |
Yes, I have tried enabling warnings around add_subdirectory(LLVM), but LLVM suppresses some warnings again, causing the enabling of warnings to be ineffective. Like:
-wd4146 # Suppress 'unary minus operator applied to unsigned type, result still unsigned'
-wd4244 # Suppress ''argument' : conversion from 'type1' to 'type2', possible loss of data'
-wd4267 # Suppress ''var' : conversion from 'size_t' to 'type', possible loss of data' |
@ggladilo @lmielick @andrey-golubev @AlexandruLorinti , could you help to review this again? The PR is to open suppressed warnings by remove some compiler flags (which make we can not see warnings in log). And we tried on vpux-plugin repo side, append cmake flag on that level can not work since llvm append flag after that again and invalid our new added flags. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShaojieZhuIntel I'm sorry, but I would like to ask you to wait until the LLVM19 update is complete.
Who should I speak to regarding this? I want to figure out whether we care enough about not-our thirdparty to have this mess forever.
@ShaojieZhuIntel but what does this do then? npu-plugin-llvm/llvm/cmake/modules/HandleLLVMOptions.cmake Lines 749 to 754 in dfee500
Do we need our custom flag at all here? Can we instead use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking for now: let's figure our what already present LLVM_ENABLE_WARNINGS
does
Signed-off-by: Zhu, Shaojie <[email protected]>
First, we need to address the binskim issue caused by this thirdparty because the target npu_llvm_utils in VPUX is related to this thirdparty. Binskim shows |
# Promoted warnings to errors. | ||
-we4238 # Promote 'nonstandard extension used : class rvalue used as lvalue' to error. | ||
) | ||
endif(NOT CLANG_CL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add these in NPU CMake to CMAKE_CXX_FLAGS instead?
something tells me it should be possible to do. and we won't need the patch here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(now that we know that ENABLE_WARNINGS works, perhaps we could move it outside?)
Summary
JIRA ticket
Related PR in NPU Compiler and/or OpenVINO repository with sub-module update
Other related tickets